Thank you @flemmons for the updates! Unfortunately, this branch has conflicts with the main branch that must be resolved in order to be merged. I will wait for the conflicts to be resolved and for the workflow check to be performed such that ToolAnalysis is confirmed to compile before I review the actual changes to the files

Conflicts were just Factory and Unity listing different added tools. I've cleared them.

Okay, but could you still resolve those conflicts? Also, it's great that ToolAnalysis can compile in your workspace, but it would be better to get confirmation that ToolAnalysis can compile in a general workspace (on GitHub via workflow). Given that this PR is self contained, there is no reason to not resolve these conflicts. I will look at the other files in the meantime.

It would be helpful to 'resolve' those comments that have been addressed to make it clearer for reviewers what is still outstanding, without having to go back through the code again.

I'm not sure I agree that the remaining issues are all analysis-specific.

• This comment suggests that the toolchain won't be writing out correct true q2 values, which seems important to me and could very well be difficult to track down if you're not aware of the reason.
• RecoCluster::CalcCB() still seems to have a placeholder value of 123 with a comment //FIXME: Need a method to have the 123 be equal to the number of operating detectors - that should be simple enough to retrieve from the geometry (ANNIEGeometry::GetNumPMTs or similar). CalcCB is called by CalcParameters which is called by the ClusterSearcher Tool, so it seems like this information is not an irrelevant side-effect if incorrect.
  • (The same issue is also in HitCleaner::CBCheck, and while that seems less important as all it does is print things, my feeling is that if the code isn't functional it shouldn't be merged, so either delete CBCheck or correct it.)
• this comment as mentioned seems like a bug that would omit a channel. I haven't run the code to check that, but nor is there any comment to say it doesn't. If it does, this is a bug I feel is worth fixing before merge.
• I know we're about to merge Yue's code that has several memory leaks but we shouldn't make a habit of it. Please address this one.

regarding this discussion on the output file from NeutronCheck, I'm inclined to agree with Franklin. Generating debug files is no bad thing - it allows focused validation and debugging, without polluting general-purpose analysis outputs. Unless these variables are likely to be useful for analysis, I would keep them where they are.
In general, I also agree that the PhaseIITreeMaker is not a good general-purpose analysis writer. It was originally a hack Tool to allow quick generation of ROOT plots. It now makes a tree with potentially 334 branches and counting. That's absurd! It has to be updated in-sync with every other Tool that adds or modifies what goes into it. The Tool, and its output files, will only grow as more Tools add to it. I don't personally consider it maintainable, or an appropriate way to work.
At minimum we could put a Tree in the DataModel and allow Tools to add branches to it, and have a simple Writer tool at the end of the Toolchain that calls TTree::Fill. That way at least each Tool remains responsible for its content, and the output file reflects what Tools were in the ToolChain, keeping content relevant to the analysis as per Franklin's request.
But of course the ANNIEEvent BoostStore is intended to be ANNIE's official output, and I'm really not sure why there's such an avoidance of BoostStores in favour of the PhaseIIColossalTreeMaker output instead....

I don't mean to add further issues, and this is probably not the right place for it, but I can't help but notice that:

• ClusterSearcherConfig
• DigitBuilderConfig
• MCRecoEventLoaderConfig

all have a config variable isMC. While setting 3 variables isn't an especially big burden, it would seem more sensible that the appropriate data reader Tool sets a flag in the DataModel and Tools check that, rather than requiring multiple flags be appropriately set for the given input files.

Regarding these [https://github.com//pull/365#issuecomment-4000879256] :
-I've changed the genie version variable to 1, which seems to be used in other toolchains. Although I will mention that my ToolAnalysis cannot find the files for these, so it can't run with this setting. I haven't checked the trueQ2 output. Seeing as how the only thing trueQ2 is used for is direct output to the file in NeutronCheck, I do not feel that this should hold up the PR any further.
-The charge balance constant is a known issue, but it's not as quick a fix as I'd like, since I'm hoping to integrate PMT masking, and make it reactive to the actual number of currently active PMTs. I will need to pass that information in from other tools. I will do this in a future update, but seeing as it's not ready to go, I'll request we let the status quo ride this PR into the next one.
-I have removed that line in the new commit.
-I'm not sure you used the correct link. What memory leak do you mean?

Thanks for the updates Franklin. That's got the vast majority of comments resolved, i think we're very nearly there.

The only one I'm still somewhat reticent to ignore is the '123' constant in the charge balance calculations.
Part of my concern is that I believed 123 to be a meaningless placeholder value that would give invalid outputs, but perhaps it's not that bad. If this is a hard-coded but correct number (based on a quick check of the HV page, ANNIE has ~119 tank PMTs active, so perhaps 123 before bad-channel masking is correct?), then that's a bit better.

While applying run-wise bad channel masking would be ideal, I'd still much prefer leaving that as a TODO but still fetching the value from geometry over hard-coding it. It feels like the distinction between "this is a simplification" and "this is sloppy", and it should be trivial to implement.

Aside from that it seems like there's a compilation error flagged by the latest CI build:

In member function 'void ClusterSearcher::RecoClusters(std::vector<RecoDigit*>*)':
UserTools/ClusterSearcher/ClusterSearcher.cpp:636:42:
error: no matching function for call to 'RecoCluster::SetDigits(std::vector<RecoDigit>&)' cluster.SetDigits(ClusteredDigits);
                                          ^
In file included from UserTools/ClusterSearcher/ClusterSearcher.h:9,
                 from UserTools/ClusterSearcher/ClusterSearcher.cpp:1:
include/RecoCluster.h:28:15: note: candidate: 'void RecoCluster::SetDigits(std::vector<RecoDigit>*)'
   inline void SetDigits(vector<RecoDigit>* indigits){fDigitList=indigits;}
               ^~~~~~~~~
include/RecoCluster.h:28:15: note:   no known conversion for argument 1 from 'std::vector<RecoDigit>' to 'std::vector<RecoDigit>*'
make[1]: [Makefile:203: UserTools/ClusterSearcher/ClusterSearcher.o] Error 1 (ignored)

Reply on slack:

According to the original phase-II design specifications, 123 is one short of the ideal case with all PMT slots full.  I haven't dug deeply into why yet, but I assume it was instituted after one tube had failed/been removed/not been installed.  There's an empty sconce in the tank, the story of which I don't know.  The 123 was put in by Michael before I got involved, and since it doesn't make much intra-run difference to the measurement (it's largely just a correction so that if all the PMTs see exactly the same charge, the CB becomes 0 for the extreme case), it's been a low priority.  That said, I do plan to fix it when I shuffle in my other neutron identification updates in the next couple of weeks.

Ok, if it's got motivation and precedent, that's good enough for me. 👍